fix: strip schema from sequence name #143
Conversation
| require 'active_record/connection_adapters/postgresql_adapter' | ||
|
|
||
| class ActiveRecord::ConnectionAdapters::PostgreSQLAdapter | ||
| include Apartment::PostgreSqlAdapterPatch | ||
| end |
There was a problem hiding this comment.
This is of course super ugly, and will crash when pg is not available, but I didn't know how to only patch this when postgresql is in use. Guidance appreciated
There was a problem hiding this comment.
For this i think we could require only when loading the postgresql schema adapter.
That being said, shouldn't we at this point simply make use of the active record reset_schema_name method when switching tenant?
There was a problem hiding this comment.
The problem with reset_sequence_name is twofold:
- It goes to the db to find it https://github.com/rails/rails/blob/ecbd92cf48c741f784c1297041d368c8eb14e9bf/activerecord/lib/active_record/model_schema.rb#L357-L360.
- Modifying the sequence name is not hread safe: Postgres sequence_name correction is not thread safe #161
Thus I believe the safest fix is to actually remove the schema from the sequence name
| def default_sequence_name(table, _column) | ||
| res = super | ||
| schema_prefix = "#{Apartment::Tenant.current}." | ||
| if res.starts_with?(schema_prefix) && Apartment.excluded_models.none?{|m| m.constantize.table_name == table} |
There was a problem hiding this comment.
I wonder if we should check Apartment.use_schemas here. Is that flag still supported?
There was a problem hiding this comment.
The flag is still in place and i don't recall having changed anything around this, so we can most likely rely on it
fsateler
force-pushed
the
bugfix/sequence-name
branch
from
7eda570
to
6644dc3
Compare
sequence_name is shared across threads because it is stored in a class ivar. This means that on threaded servers, the sequence_name might point to a different tenant, if another tenant made the switch at the right time. This race is likely the cause of rails-on-services#81 as well. This reverts commit f8eefc4.
fsateler
force-pushed
the
bugfix/sequence-name
branch
from
6644dc3
to
052e29e
Compare
| @@ -121,11 +127,9 @@ | |||
| it 'connects and resets' do | |||
| subject.switch(schema1) do | |||
| expect(connection.schema_search_path).to start_with %("#{schema1}") | |||
| expect(User.sequence_name).to eq "#{schema1}.#{User.table_name}_id_seq" | |||
There was a problem hiding this comment.
Was there anything particularly wrong with this test? Shouldn't these assumptions still be valid?
There was a problem hiding this comment.
Ok, after starting to make some tests with my codebase and looking at the changes made, I now understand.
The previous changes tried to fix this issue, but ended up introducing other bugs while fixing this problem.
So, please correct me if i'm wrong, but i think we'll need to merge these changes, that will revert the state to "broken".
Release 2.9.1.
Then we can merge the changes i'm suggesting in #169 (assuming it looks ok), document the apartment model logic and release the 3.0.0.
Agree?
There was a problem hiding this comment.
I believe this PR solves the problem better, because default_sequence_name involves a trip to the database.
There was a problem hiding this comment.
Got it. I did a couple of tests and as long as we have the correct search path set, it should be ok and we should be getting the correct sequence. Let's go ahead w your solution.
I still think that having the logic on a concern that we include in our models would be a simpler solution than trying to manage this changes only when using postgres
There was a problem hiding this comment.
I still think that having the logic on a concern that we include in our models would be a simpler solution than trying to manage this changes only when using postgres
I'd be willing to explore that but I'm not sure exactly what you have in mind. Do you mean something we extend into ActiveRecord::Base?
There was a problem hiding this comment.
Essentially something along these lines: https://github.com/rails-on-services/apartment/pull/169/files#diff-19a1d0c8bf6f60ce84cd210d933378fb77dd60a56232b05b0d2b89c1979c6331R8
I've adjusted the PR to follow your approach but with what i was suggesting
sequence_name is shared across threads because it is stored in a class
ivar. This means that on threaded servers, the sequence_name might point
to a different tenant, if another tenant made the switch at the right
time. This race is likely the cause of #81 as well.
This reverts commit f8eefc4.